-
Notifications
You must be signed in to change notification settings - Fork 498
Fix Instance() method of Singleton classes #3269
Conversation
In particular, make sure that all the Instance() methods of the same class always use the same instantiation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in reviewing this. I have one comment regarding deprecation, otherwise, LGTM!
What is your opinion regarding merging this @scpeters @azeey ? We just hit a regression involving this patch in conda-forge (see conda-forge/gazebo-feedstock#161), to avoid this in the future it would be great if we could get this eventually in an official release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, this looks like a good set of changes
I'm rerunning Ubuntu CI and then it will be ready for merge, though I may need to do some testing with ROS before releasing this
Sure, we can also just wait for the ROS tests before even merging this PR, I am not particulary in a hurry on this one. I am just afraid that at some point the problem that affected conda-forge macOS builds also started to affect other builds (for example homebrew builds as soon as a new version of Apple Clang is released). |
In particular, make sure that all the Instance() methods of the same class always use the same instantiation.
🦟 Bug fix
Fixes conda-forge/gazebo-feedstock#148 .
Summary
This should fix all the cases in which multiple instances of a Singleton classes were observed, as for example in conda-forge/macOS build of Classic Gazebo (see conda-forge/gazebo-feedstock#148 (comment)). The fix was inspired from gazebosim/gz-sim#1725 and from the related PRs:
Thanks a lot to @azeey for suggesting the correct fix in conda-forge/gazebo-feedstock#152 (comment) .
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.